Distinguish expired, refresh-failed, and absent auth sessions - #160

This repo is public, so a ticket key in a comment is a dead reference for any
reader who can't resolve it. Each of these drops the parenthetical and keeps the
sentence around it - the reasoning was the useful part, not the pointer.

One needed rewording rather than deletion, since the key was carrying the
sentence's subject.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

Several comments explained the code by narrating the incident that prompted it -
"the bug this ticket exists for", "previously absent", "the old single
no_access_token", "a machine sat in this exact shape for over a week". That
framing decays: a reader a year out has no ticket, no before-state, and no
memory of whose machine it was, and is left with a comment that describes
something no longer in the tree.

Each now states the constraint as it stands. Same reasoning, anchored to the
code instead of to a moment: why the three codes can't be interchanged, why the
refresh token is left in place, why whoami has to report the environment, and
what state the regression tests pin.

Comments and one test name only; no behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

Three comments claimed that re-running `auth login` after a refresh failure
turns a recoverable state into an unrecoverable one, and that each retry leaves
the install worse than the last. Neither is true of this code.

`login` calls `store.save` only after the code exchange and the token_info
lookup both succeed, and its catch just flushes a failure page and rethrows. So
a login either replaces the slot with a working token pair or writes nothing at
all - the prior refresh token survives an abandoned or failed attempt. Nothing
accumulates across attempts either: client creds are reused rather than
re-registered, and there is no partial write to compound.

The rule those comments guard is still right, for a different reason. A retry
after `refresh_failed` is free and the credential it needs is still on file,
while a login needs a human at a browser - which is precisely what an agent on a
headless box cannot produce, so pointing there dead-ends instead of recovering.
A successful login does invalidate the refresh token it replaces, which matters
to anything else holding that credential; it does not harm this install.

Pin the invariant the retry advice depends on: a failed login must leave an
existing session's tokens intact.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

Four review fixes, all in the taxonomy this branch introduces:

- getValidUserToken's doc comment claimed it returns null whenever the session
  can't produce a token, but it throws on a rejected refresh. A caller reduced to
  null can't tell that state from absence, which is the confusion the branch
  exists to remove.
- whoami's comment claimed to be the only place the environment is reported;
  failures now carry it in error.environment too. Scoped to success.
- sessionFailure's comment named `refresh_failed` alongside two real wire codes,
  but that one is the internal outcome kind - the wire code is
  token_refresh_failed.
- no_company_uuid set the environment field while its message never named it.
  Human-mode output prints the message and never that field, so the environment
  was invisible to half the callers for an error whose own comment calls it
  decisive.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

A 401 fell through to the generic 4xx bucket: code api_client_error, exit 4,
message just the request line. Same code a 422 gets, so nothing distinguished
"your credential is no good" from "your request is malformed", and none of the
auth taxonomy applied - no environment, no guidance, and an exit code that a
caller branching on 3 for credential trouble never sees.

It now exits Auth as credential_rejected, alongside the 403 insufficient_scope
case that has always classified this way. The message names which credential was
refused, because the recovery differs: a stored session can be signed in again,
while a token supplied through GUSTO_ACCESS_TOKEN or --token-stdin is the
caller's to fix and must not be answered with `auth login`, which would rotate a
session the failing command never used. Nothing re-authenticates a rejected token
on its own, so no wording suggests a bare retry - the opposite of
token_refresh_failed, where the retry is the whole point.

The client stamps its credential onto the ApiError it throws, the way it already
does for requestId, so every toResult caller reports a 401 identically without
threading context through call sites several frames from a resolved context.

Behavior change: a 401 exits 3 where it used to exit 4. Two tests asserted the
old classification and now assert the new one. README's exit-code line said 4 for
all API 4xx, which was already untrue of the 403 scope case; it now says what
decides it.

Not included: refreshing a rejected token and retrying the request. It only helps
where the credential is recoverable, and a server-side revocation still fails -
whereas correct classification helps every case. The reactive refresh path in
withUserToken remains unwired.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

The within-skew failure path said "the 401 path refreshes later if needed",
deferring to `withUserToken`. Nothing reaches that function, so the deferral
described a recovery that never happens: the token goes out, comes back 401, and
is reported rather than refreshed. Say that, and name where the 401 lands.

The neighboring short-circuit justified itself with "a 401 whose message says
nothing about why", which classifying 401s made untrue. The reason to name the
state locally is now that we already know it, and only the local state can date
the expiry and name the slot - not that the alternative is uninformative.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

Six comments explained the change being made rather than the code being left
behind, which reads fine in review and badly a year later:

- The within-skew passthrough cited the absence of a caller for withUserToken.
  That is a fact about today's call graph, and wiring one up would make the
  comment wrong while the behavior it describes stayed the same. Says there is no
  reactive refresh, and what that costs.
- The expiry short-circuit compared itself to how a 401 used to read. Compares
  the two reports on what each can say instead.
- The credential on ApiError, its client option, and the type itself justified
  where they live against the alternative of threading a parameter. Nobody
  maintaining this needs the road not taken; they need to know the credential
  rides on the error so distant callers can name it.
- The end-to-end 401 test defended its own existence against the unit test.
  Now says what it guards: a hand-built ApiError satisfies toResult whether or
  not the wiring that fills it in still holds.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

The persisted `environment` was installed through module state in
global-flags.ts and a `setConfiguredEnvironment` setter called from
main(), so that a synchronous `readGlobalFlags` could reach it. Commander
already resolves this: an explicit `--env` outranks `.env("GUSTO_ENVIRONMENT")`,
which outranks `.default()`. Read the config before building the program
and pass it in as that default.

Drops the mutable module state, the test-only setter, and the per-test
restore discipline it needed. `readGlobalFlags` goes back to passing the
resolved value through, and `defaultEnv` still owns the production
fallback - the default is only installed when one is actually persisted,
so an unset `--env` stays undefined.

The precedence chain was already covered end to end through the compiled
binary in tests/smoke.test.ts, which is where it belongs now that
commander owns it; the unit tests of the setter go away.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

Stamping `auth` onto the error made the constructor six positional
params, two of them optional trailers that read identically at a call
site. Group `requestId` and `auth` into a context object; the four
required args stay positional.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

`token_refresh_failed` told every caller to "retry the command first,
since the retry is free". That holds when the server rejected the
attempt, but `invalid_grant` - the reason a revoked or expired refresh
token actually comes back with, and the common case - is a verdict on
the token, so the retry re-runs the same refresh and fails identically.
The advice was wrong for the state it fires on most.

Split the message on the reason: `invalid_grant` names the token as
rejected and points at `auth login`, noting that the credential the
login replaces is already dead, so replacing it costs nothing. Every
other reason (a 5xx, `temporarily_unavailable`, a fetch fault) keeps
the retry-first wording it was written for. The code is unchanged -
what differs is the recovery, not the state a caller branches on.

`credential_rejected` had a quieter version of the same problem: for a
stored session it sent the caller to `auth login` without saying that
this replaces the slot's refresh token, which a 401 does not prove is
bad - notably in a slot with no recorded `expiresAt`, where nothing
refreshed proactively because nothing knew to. It still points there,
since a rerun re-sends the same rejected token and refreshing a
rejected credential in place is `withUserToken`'s unwired job, but it
now says what the login spends.

Verified against a local token-endpoint stub: `invalid_grant` gets the
login wording, `temporarily_unavailable` gets the retry wording, and
the stored refresh token survives both.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

After `resolveSessionToken` landed, `getValidUserToken` was an adapter
that flattened three failure kinds into null-or-throw, and its own doc
comment pointed callers at `resolveSessionToken` instead. It has no
production callers - neither does `withUserToken`, which is parked for
reactive refresh - so the adapter existed only to be tested. Inline it.

Its one test the union's own tests didn't already cover (a successful
within-skew refresh persisting the new pair) moves to the
`resolveSessionToken` block.

Also states the login-is-expensive invariant once, on the union that
encodes it, rather than re-deriving it at each site that reports one of
these states; `refresh_failed` now records that whether its refresh
token is still usable depends on `cause`.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>

Conflict was the api-context.ts import block only. Resolved as the union
of both sides, minus `getValidUserToken`, which this branch inlined into
`withUserToken` and deleted. `OAuthError` goes back to a type-only
import: main needed it as a value for an `instanceof` check in the old
`sessionToken`, which `resolveSessionToken` now owns.

main's new `putResourceWithVersion` resolves through `resolveApiContext`,
so it picks up the auth-context stamping and reports a 401 as
`credential_rejected` with no extra wiring. Its two clarification legs
compose with that: `clarifyVersionConflict` gates on `ExitCode.ApiClient`
so an auth failure passes through untouched, and
`clarifyVersionReadFailure` spreads the underlying error, keeping the
code, exit, and `environment` while explaining nothing was written.

Signed-off-by: Jeff Stephens <jeff.stephens@gusto.com>